Skip to content

fix(cli): stop housekeeping hanging on a deleted bind mount - #11285

Merged
doudouOUC merged 2 commits into
QwenLM:mainfrom
doudouOUC:fix/mkdirp-stale-mount-hang
Sep 7, 2026
Merged

doudouOUC merged 2 commits into
QwenLM:mainfrom
doudouOUC:fix/mkdirp-stale-mount-hang

Conversation

@doudouOUC

Copy link
Copy Markdown
Collaborator

What this PR does

Housekeeping ensures its lock directory exists before taking the once-per-interval lock. That call asked for a recursive create; this PR makes it create a single level instead, so a create that cannot succeed fails fast rather than retrying forever.

Why it's needed

When the global qwen dir lives inside a bind mount whose source directory has since been deleted, the mountpoint still stats as a directory while creating entries inside it returns ENOENT. Node's recursive mkdir interprets that ENOENT as "the parent is missing", creates the parent (EEXIST), stats it to confirm it is a directory, retries the leaf, gets ENOENT again — with no retry cap and no backoff. The returned promise therefore never settles, which has two consequences: the existing .catch(() => {}) never runs, so nothing is logged and nothing degrades gracefully; and the await wedges the housekeeping chain for the remaining lifetime of the process.

This is not theoretical — it was the amplifier behind a CPU overload on the self-hosted ECS runner pool. E2E sandbox containers that outlived their host-side docker run client kept running after their job had already deleted the workspace those containers had bind-mounted. Roughly a minute later each container's housekeeping timer fired, hit this path, and started spinning. Measured on one 64-core host: 177 such processes, ~2.5k failing mkdir/s each, ~67% system time fleet-wide, and 45 CPU-hours accumulated by the oldest single process. Across five hosts the orphaned containers were consuming about 232 of 320 cores. Reaping the containers dropped one host's load average from 213 to 12.

Container leakage itself is being addressed separately in #11264; this PR removes the CLI-side amplifier so that an orphaned or otherwise wedged sandbox costs approximately nothing instead of a full core.

A single level is sufficient here: all three runThrottledOnce call sites put lockPath directly in the global qwen dir, whose own parent is $HOME. When the directory genuinely cannot be created the subsequent lock acquisition surfaces the error, so housekeeping degrades to a skip — the intended behaviour — instead of hanging.

Reviewer Test Plan

How to verify

The unit tests cover the invariant and the preserved behaviour: cd packages/cli && npx vitest run src/utils/housekeeping/throttledOnce.test.ts. Two tests are added — one asserts the lock directory is still created (with no group/other permission bits) when it does not exist yet, and one asserts no mkdir call requests recursive: true. To confirm the second test is a real guard rather than a tautology, temporarily restore recursive: true in throttledOnce.ts; it fails with expected { recursive: true, mode: 448 } to not match object { recursive: true }.

To reproduce the underlying condition on Linux (needs root for mount; unshare -m keeps the bind mount in a private namespace so nothing leaks to the host):

unshare -m bash -c '
  B=/tmp/verify.$$; mkdir -p "$B/src/qhome" "$B/mnt"
  mount --bind "$B/src" "$B/mnt"
  rm -rf "$B/src"    # source deleted; mountpoint is now //deleted
  node -e "
    const fs = require(\"fs/promises\");
    const t = process.argv[1];
    let a = null, b = null;
    fs.mkdir(t, { recursive: true, mode: 0o700 }).then(() => (a = \"RESOLVED\"), e => (a = \"REJECTED \" + e.code));
    fs.mkdir(t, { mode: 0o700 }).then(() => (b = \"RESOLVED\"), e => (b = \"REJECTED \" + e.code));
    setTimeout(() => { console.log(\"recursive:true  ->\", a ?? \"STILL PENDING\"); console.log(\"recursive:false ->\", b ?? \"STILL PENDING\"); process.exit(0); }, 6000);
  " "$B/mnt/qhome"
  umount "$B/mnt"; rm -rf "$B"
'

Expected: the recursive form is still pending after six seconds while the single-level form has already rejected with ENOENT. Wrapping each form in /usr/bin/time shows the recursive form burning 103% CPU against 0% for the single-level form.

Evidence (Before & After)

Not a user-visible or TUI change. Behavioural evidence is the reproduction above, measured on one of the affected Linux hosts:

mountinfo: /tmp/verify.2928891/src//deleted /tmp/verify.2928891/mnt ext4

before (recursive:true)   ->  *** STILL PENDING (never settles) ***   cpu=103%
after  (recursive:false)  ->  REJECTED ENOENT                        cpu=0%

For reference, the syscall signature observed on a live wedged process before the fix — mkdir failing 100% of the time, at roughly 2.5k calls per second:

% time     calls    errors  syscall
 84.27    135267      9788  futex
  9.51     76112            epoll_pwait
  1.53     25428     25428  mkdir
  0.96     12714            statx

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux

macOS: unit tests — throttledOnce.test.ts 13 passed, and the full housekeeping surface (src/utils/housekeeping plus src/services/housekeeping) 80 passed. Linux: the bind-mount reproduction and before/after measurement above. Windows: not tested; the failure mode is Linux bind-mount specific, and the change itself is platform-neutral.

Environment (optional)

Unit tests only, plus a root shell on an affected Linux host for the mount-level reproduction.

Risk & Scope

  • Main risk or tradeoff: if a caller ever passes a lockPath whose parent directory does not already exist, the directory is no longer created for it and housekeeping skips that pass instead of silently bootstrapping the tree. All three current call sites resolve to the global qwen dir, so none are affected; a new call site with a deeper path would need to create its own parent.
  • Not validated / out of scope: I could not explain why the same never-settling call presents as a hot mkdir loop in some processes (production, and a standalone Node repro at 103% CPU) but as a quiet hang with no mkdir syscalls at all in others (the real CLI under my reproduction — promise still pending, 0% CPU, idle libuv workers). Both variants are removed by this change, but the trigger that selects between them is unknown; a plausible but unverified factor is the extra nested bind mount present in the CI layout. Container leakage itself is out of scope and handled by fix(ci): reap leaked E2E sandbox containers #11264. No Windows verification.
  • Breaking changes / migration notes: none.

Linked Issues

Related to #11264, which stops the E2E sandbox containers from leaking in the first place. This PR is independent and complementary: it removes the CLI-side cost of a container that has already been orphaned, whatever the cause.

中文说明

这个 PR 做了什么

housekeeping 在获取「每周期一次」的锁之前,会先确保锁目录存在。原先这个调用用的是递归创建;本 PR 改为只创建一层,这样当创建注定无法成功时会快速失败,而不是无限重试。

为什么需要

当全局 qwen 目录位于某个 bind mount 内、而该挂载的源目录已被删除时,挂载点本身仍然能被 stat 成目录,但在其内部创建条目会返回 ENOENT。Node 的递归 mkdir 把这个 ENOENT 理解为「父目录缺失」,于是去创建父目录(得到 EEXIST)、stat 确认它是目录、重试叶子节点、再次得到 ENOENT —— 没有重试上限,也没有退避。因此返回的 promise 永远不会 settle,带来两个后果:现有的 .catch(() => {}) 永远不会执行,既没有日志也没有优雅降级;同时这个 await 会把整条 housekeeping 链卡死到进程生命周期结束。

这不是理论推演 —— 它正是自建 ECS runner 集群一次 CPU 过载的放大器。E2E sandbox 容器在宿主机侧的 docker run 客户端被回收后仍在运行,而对应 job 早已删除了这些容器 bind mount 进去的工作目录。大约一分钟后,每个容器的 housekeeping 定时器触发、命中这条路径并开始空转。在一台 64 核宿主机上实测:177 个这样的进程,每个每秒约 2500 次失败的 mkdir,全机 system time 约 67%,其中最老的单个进程累计消耗 45 CPU 小时。五台宿主机上,这些孤儿容器合计占用了约 320 核中的 232 核。回收容器后,其中一台的 load average 从 213 降到 12。

容器泄漏本身由 #11264 单独处理;本 PR 移除 CLI 侧的放大器,使得一个已经变成孤儿或以其它方式卡死的 sandbox 的开销接近于零,而不是吃满一个核。

这里只创建一层就足够了:runThrottledOnce 的三个调用点都把 lockPath 直接放在全局 qwen 目录下,而该目录的父目录是 $HOME。当目录确实无法创建时,后续的加锁步骤会把错误暴露出来,于是 housekeeping 降级为跳过 —— 这正是预期行为 —— 而不是挂死。

评审验证方案

如何验证

单元测试覆盖了这个不变量以及被保留的行为:cd packages/cli && npx vitest run src/utils/housekeeping/throttledOnce.test.ts。新增两个测试 —— 一个断言锁目录在不存在时仍会被创建(且不带 group/other 权限位),另一个断言没有任何 mkdir 调用请求 recursive: true。为了确认第二个测试是真正的护栏而不是同义反复,可以临时把 throttledOnce.ts 里的 recursive: true 加回去;它会失败并报出 expected { recursive: true, mode: 448 } to not match object { recursive: true }

在 Linux 上复现底层条件(mount 需要 root;unshare -m 把 bind mount 隔离在私有命名空间内,不会泄漏到宿主机):使用上面英文部分给出的脚本。

预期结果:递归形式在六秒后仍处于 pending,而单层形式已经以 ENOENT 拒绝。用 /usr/bin/time 分别包裹两种形式可以看到,递归形式烧掉 103% CPU,单层形式为 0%

证据(前后对比)

不是用户可见或 TUI 变更。行为证据即上面的复现,在一台受影响的 Linux 宿主机上实测:改动前(recursive:true)六秒后仍未 settle、CPU 103%;改动后(recursive:false)以 ENOENT 拒绝、CPU 0%。作为参考,修复前在一个活体卡死进程上抓到的系统调用特征是 mkdir 100% 失败、每秒约 2500 次,详见英文部分表格。

测试平台

macOS:单元测试 —— throttledOnce.test.ts 13 项通过,housekeeping 全量(src/utils/housekeepingsrc/services/housekeeping)80 项通过。Linux:上述 bind mount 复现与前后对比测量。Windows:未测试;该故障模式是 Linux bind mount 特有的,而改动本身与平台无关。

环境(可选)

仅单元测试,另加一台受影响 Linux 宿主机上的 root shell 用于挂载层面的复现。

风险与范围

  • 主要风险或权衡:如果将来有调用方传入父目录尚不存在的 lockPath,该目录将不再被自动创建,housekeeping 会跳过这一轮而不是静默地把整棵目录树补齐。当前三个调用点都指向全局 qwen 目录,因此都不受影响;若新增更深路径的调用点,需要自行创建其父目录。
  • 未验证 / 超出范围:我无法解释为什么同一个永不 settle 的调用,在部分进程中表现为 mkdir 热循环(生产环境,以及一个 103% CPU 的独立 Node 复现),而在另一些进程中表现为安静挂起、完全没有 mkdir 系统调用(真实 CLI 在我的复现下 —— promise 仍 pending、CPU 0%、libuv worker 全空闲)。本改动会消除这两种表现,但决定走向哪一种的触发条件尚不清楚;一个合理但未经验证的猜测是 CI 布局中多出的一层嵌套 bind mount。容器泄漏本身超出本 PR 范围,由 fix(ci): reap leaked E2E sandbox containers #11264 处理。没有 Windows 验证。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

#11264 相关,后者从源头阻止 E2E sandbox 容器泄漏。本 PR 与其独立且互补:无论成因如何,它消除的是一个已经变成孤儿的容器在 CLI 侧带来的开销。

`runThrottledOnce` ensured its lock directory with
`mkdir(..., { recursive: true })`. When the global qwen dir sits inside a
bind mount whose source directory has been deleted, the mountpoint still
stats as a directory while creating entries inside it returns ENOENT.
Node's recursive mkdir reads that ENOENT as "parent is missing", creates
the parent (EEXIST), confirms it is a directory, retries the leaf, gets
ENOENT again — with no retry cap. The returned promise never settles, so
the existing `.catch` never runs and the `await` wedges the housekeeping
chain for the rest of the process lifetime.

CI hit this on the self-hosted pool: e2e sandbox containers that outlived
their host-side `docker run` client kept running after the job had
already deleted their workspace, and each one then burned a core issuing
~2.5k failing mkdir/s. Measured on one host: 177 such processes, 45 CPU
hours accumulated by the oldest.

A single-level mkdir is enough — all three callers put `lockPath`
directly in the global qwen dir, whose own parent is $HOME — and it fails
fast with ENOENT instead of spinning, so housekeeping degrades to a skip.
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every section filled in, including a real before/after and the platform table.

Problem: observed, not theoretical, and about as well-evidenced as a fix gets. There is a production incident with measurements (177 wedged processes, ~2.5k failing mkdir/s each, ~67% system time fleet-wide, one host's load average 213 → 12 after the containers were reaped), an strace syscall signature taken from a live wedged process, and a self-contained unshare -m bind-mount reproduction showing the recursive form still pending at 6s / 103% CPU against the single-level form rejecting ENOENT at 0%. I checked the cross-reference too: #11264 is open and does address the container leak itself, so this is genuinely the complementary CLI-side half rather than a duplicate of it.

Direction: aligned. The real defect is that mkdir(..., { recursive: true }) has no retry cap and no backoff, so on a mountpoint that still stats as a directory but rejects creates with ENOENT, the returned promise never settles. That defeats the .catch(() => {}) sitting right beside it — the guard that was supposed to make this best-effort can never run — and the await wedges the housekeeping chain for the remaining life of the process. Turning an intended graceful skip into a permanent hang plus a burned core is squarely a bug, not hardening.

Size: not applicable — packages/cli/src/utils/housekeeping/ is not a core path, and this is a single package. For the record: 17 production lines (14 added + 3 removed) in one file, 38 test lines, 2 files total. Well under every threshold in either tier.

Approach: the scope feels right and I'd have proposed the same thing. I weighed the two alternatives before reading the diff — wrapping the mkdir in a timeout, or pre-statting the parent — and both are worse here. A timeout lets the awaiting chain move on but cannot cancel the retry loop Node is already running, so the CPU cost that made this an incident may well survive it; pre-statting adds code without removing the unbounded retry at all. Dropping recursive: true addresses both symptoms in one line, which is the minimal change for the stated goal. Nothing unrelated rides along in the diff.

One caveat to flag now and detail in the code review: the description justifies one level with "every caller puts lockPath directly in the global qwen dir, whose own parent is $HOME". The first half is exactly right — I verified all three call sites. But the directory this mkdir creates is the global qwen dir itself, and QWEN_HOME can point that somewhere deeper than $HOME/.qwen, so the "parent is $HOME" half is narrower than it reads. Non-blocking, and the failure mode stays graceful — details below.

Risk: no elevated risk signals. Neither changed file matches the revert-correlated path list.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 每个部分都填写了,包括真实的 before/after 和平台表格。

问题: 是已观测到的问题,不是理论推演,而且证据充分得少见。有带实测数据的生产事故(177 个卡死进程,每个每秒约 2500 次失败的 mkdir,全机 system time 约 67%,回收容器后其中一台的 load average 从 213 降到 12),有从活体卡死进程上抓到的 strace 系统调用特征,还有一个自包含的 unshare -m bind mount 复现:递归形式在 6 秒后仍 pending、CPU 103%,而单层形式已经以 ENOENT 拒绝、CPU 0%。我也核对了交叉引用:#11264 处于 open 状态,处理的正是容器泄漏本身,所以本 PR 确实是互补的 CLI 侧那一半,而不是它的重复。

方向: 对齐。真正的缺陷在于 mkdir(..., { recursive: true }) 既没有重试上限也没有退避,所以在「仍能 stat 成目录、但创建条目会返回 ENOENT」的挂载点上,返回的 promise 永远不会 settle。这使得紧挨着它的 .catch(() => {}) 失效——本该让这一步变成 best-effort 的兜底永远不会执行——同时这个 await 会把整条 housekeeping 链卡死到进程结束。把预期中的优雅跳过变成永久挂死外加吃满一个核,这是明确的 bug,不是加固。

规模: 不适用 —— packages/cli/src/utils/housekeeping/ 不属于核心路径,且只涉及单个 package。记录一下:一个文件里 17 行生产代码(新增 14 + 删除 3),38 行测试,共 2 个文件。远低于两档门槛中的任何一个。

方案: 范围合理,我自己也会提出同样的做法。在看 diff 之前我权衡过另外两条路——给 mkdir 包一层超时,或者先 stat 父目录——在这里都更差。超时能让等待的一方继续走下去,但无法取消 Node 已经在跑的重试循环,所以真正酿成事故的那部分 CPU 开销很可能依然存在;先 stat 父目录则只是增加代码,完全没有消除无上限重试。去掉 recursive: true 用一行同时解决了两个症状,这正是达成目标所需的最小改动。diff 里也没有夹带无关内容。

有一点现在先提出、代码审查里再展开:描述用「每个调用方都把 lockPath 直接放在全局 qwen 目录下,而该目录的父目录是 $HOME」来论证只需一层。前半句完全正确——我核对了全部三个调用点。但这个 mkdir 创建的目录就是全局 qwen 目录本身,而 QWEN_HOME 可以把它指向比 $HOME/.qwen 更深的位置,所以「父目录是 $HOME」这半句比读起来要窄。这不构成阻塞,且失败模式仍然是优雅的——详见下文。

风险: 无升级风险信号。两个改动文件都不匹配与 revert 相关的路径列表。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 4d54e6a3364a462f046d424120ded6a9ba18f667 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Code review

I traced the change against every consumer rather than taking the description's word for it, since dropping recursive: true is only safe if the directory being created never needs a parent bootstrapped.

The load-bearing claim holds. dirname(opts.lockPath) resolves to the global qwen dir itself for all three call sites, not to a subdirectory of it — so one level is indeed the whole job:

Call site lockPath dirname
file-history-cleanup join(qwenDir, FILE_HISTORY_MARKER + '.lock') qwenDir
subagent-cleanup getSubagentMarkerPath(qwenDir, projectDir) + '.lock', which is join(qwenDir, SUBAGENT_MARKER-<hash>) + '.lock' qwenDir
openai-logs-cleanup getOpenAILogsMarkerPath(qwenDir, logDir) + '.lock', which is join(qwenDir, OPENAI_LOGS_MARKER-<hash>) + '.lock' qwenDir

The two hashed marker paths were the ones worth checking — both hash the key into a filename, not into extra directory levels, so neither reaches below qwenDir.

The failure path degrades instead of wedging, on both schedulers. This is what makes the change safe rather than just smaller. If the mkdir now fails, .catch(() => {}) swallows it, tryAcquire hits open(lockPath, 'wx'), gets ENOENT, and rethrows — only EEXIST is converted to false. That throw is caught in both entry points: runPass logs housekeeping pass failed; will retry next cycle and still schedules the next pass, and the non-interactive worker's catch logs and reschedules via NON_INTERACTIVE_FAILURE_RETRY_MS. So the timer chain survives either way, which matches what the PR intends.

One non-blocking note — the "parent is $HOME" half of the justification is narrower than it reads. getGlobalQwenDir() returns Storage.resolvePath(process.env.QWEN_HOME) when that variable is set, falling back to path.join(os.homedir(), QWEN_DIR). So the parent of the directory this mkdir creates is $HOME only in the default layout; with QWEN_HOME=/some/deep/path whose parent chain doesn't exist yet, the old recursive call would have bootstrapped the tree and the new one won't. The Risk section already gestures at this but frames it as a hypothetical future caller passing a deeper lockPathQWEN_HOME makes the same gap reachable today without any new call site. Worth a wording tweak rather than a code change: the outcome is still the intended graceful skip (logged failure, retry next cycle, chain intact), housekeeping is best-effort cleanup, and I found no other production code that recursively creates the global qwen dir at startup — the only recursive creator is PollingChannelBase, and that's getGlobalQwenDir()/channels, conditional on a polling channel actually running.

The comment block earns its place. Ten lines of comment on a one-line change would normally be too much, but the why here is genuinely non-obvious and load-bearing: without it the next contributor re-adds recursive: true as an obvious-looking robustness improvement and silently reintroduces the hang. The pre-existing comment above it was extended rather than replaced, which is the right call.

Tests are real guards, not decoration. The first new test uses a fresh directory that genuinely doesn't exist, so it pins that dropping recursive didn't lose the create — and asserting stat.mode & 0o077 === 0 instead of an exact mode keeps the 0o700 convention check umask-proof, which is a nice touch. The second iterates every mkdir call and rejects recursive: true; the author demonstrated it fails with a specific matcher message when the option is restored, so it's falsifiable rather than tautological.

Neither test pins the incident itself — a never-settling promise on a deleted bind mount needs root and a mount namespace, which is reasonably out of scope for a unit test. That gap is what the verification line below is for.

No reuse concerns: nothing new is being abstracted, so there's no parallel utility to consolidate.

Test evidence

Update: CI settled after this section was first written, and it settled with the unit suite dead on arrival. The original version of this comment reported the Linux unit job as in progress; the table and the analysis below are the settled state for commit 4d54e6a3364a462f046d424120ded6a9ba18f667. This is an unattended CI run, so I did not build, run, or execute anything from this PR — all of it was read through the API. Of 55 check-runs: 16 success, 2 failure, 34 skipped, and 3 still in progress (the bot's own review-pr and triage orchestration jobs, which run on pull_request_target and are not this PR's CI).

Final CI results for 4d54e6a (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ❌ failure
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
OpenTUI no-flicker gate ✅ success
route ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
TUI parity snapshots (ink vs opentui) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Both red checks are runner-side checkout failures, not this PR. They are the same class of failure, and neither reached any of the PR's code:

  • Classify PR failed in its Checkout trusted CI classifier step — the job's only non-success step — with The process '/usr/bin/git' failed with exit code 128.
  • Test (ubuntu-latest, Node 22.x) failed in its Checkout step, after three attempts, with fatal: unable to access 'https://github.com/QwenLM/qwen-code/': Failed to connect to github.com port 443 after 134983 ms: Couldn't connect to server. The test step never started.

A two-file change under packages/cli/src/utils/housekeeping/ cannot make a self-hosted runner unable to reach github.com:443. I reached that call from the failing step identities and the git/network error text — both runner-generated — not from anything in the log body.

But "infra noise" is not the same as "evidence exists", and that distinction decides the verdict here. The one job that would have executed this PR's two new tests died before running a single one, and the macOS and Windows Test matrices are skipped on this PR. So no platform, in CI, has ever run creates the lock directory when it does not exist yet or never asks for a recursive mkdir. What did pass is real and worth having — Lint & Static (so typecheck and ESLint are clean on the changed file), Integration Tests (no-AK, No Sandbox), the OpenTUI no-flicker gate, both Desktop Shell jobs, and TUI parity snapshots — but none of those touch throttledOnce. The new tests are currently unexecuted anywhere except the author's own machine.

Not verified, and stated plainly:

  • The two new unit tests have no CI result. The Linux job failed at Checkout; macOS/Windows are skipped. The only numbers that exist for them are the author's (throttledOnce.test.ts 13 passed, housekeeping surface 80 passed, on macOS) — the author's claim, not evidence, and not re-run here.
  • The bind-mount reproduction is the author's evidence, not independently re-run. The before/after numbers (recursive still pending at 6s / 103% CPU vs. single-level ENOENT at 0%), the strace signature, and the fleet measurements (177 processes, ~2.5k failing mkdir/s, load average 213 → 12) all come from the PR description. Reproducing them needs root and a mount namespace, and this run neither executes PR-derived code nor has that environment.
  • Windows is untested — the author marks it ⚠️ and reasons the failure mode is Linux-bind-mount specific while the change itself is platform-neutral. That reasoning looks sound (mkdir without recursive is not a platform-specific API), and with the Windows Test matrix skipped nothing here confirms it either way.
  • The author's own open question stays open: why the same never-settling call presents as a hot mkdir loop in some processes and a quiet hang with no mkdir syscalls in others. The PR removes both variants either way, so this doesn't block the change — but it's an honest unknown, correctly labelled as one rather than explained away.

Sandboxed verification would settle the one claim that currently rests on the author's word: @qwen-code /verify — that mkdir(..., { recursive: true }) genuinely fails to settle on a mountpoint whose source was deleted, and that removing the option makes it reject fast, A/B against the base build. This PR's own suite pins the invariant (no mkdir call asks for recursive: true) but not the mechanism that makes the invariant matter, and a mount-namespace repro is exactly the kind of thing static review cannot substitute for. It would also, incidentally, produce the first actual execution of the two new tests.

中文说明

代码审查

我是顺着每一个调用方去核对这个改动的,而不是直接采信描述里的说法——因为去掉 recursive: true 只有在「被创建的目录永远不需要顺带补齐父目录」时才安全。

关键论断成立。 对全部三个调用点来说,dirname(opts.lockPath) 解析出来的就是全局 qwen 目录本身,而不是它的某个子目录——所以「只创建一层」确实就是全部工作量。上面表格列了三处的 lockPath 与对应的 dirname。两个带哈希的 marker 路径是最值得核对的:它们都是把哈希算进文件名、而不是算进额外的目录层级,所以都不会伸到 qwenDir 下面去。

失败路径是降级而不是卡死,两个 scheduler 都是如此。 这才是让这个改动「安全」而不只是「更小」的原因。如果 mkdir 现在失败了,.catch(() => {}) 会把它吞掉,接着 tryAcquire 走到 open(lockPath, 'wx')、拿到 ENOENT、然后重新抛出——只有 EEXIST 会被转成 false。这个抛出在两个入口都被接住了:runPass 记录 housekeeping pass failed; will retry next cycle 并照常安排下一轮,非交互 worker 的 catch 则记录日志并通过 NON_INTERACTIVE_FAILURE_RETRY_MS 重新排程。所以定时器链在两种情况下都能存活,这与 PR 的意图一致。

一条不阻塞的提醒——「父目录是 $HOME」这半句论证比读起来要窄。 getGlobalQwenDir() 在设置了 QWEN_HOME 时返回 Storage.resolvePath(process.env.QWEN_HOME),否则才回落到 path.join(os.homedir(), QWEN_DIR)。也就是说,只有默认布局下这个 mkdir 所创建目录的父目录才是 $HOME;如果 QWEN_HOME=/some/deep/path 而其父级路径链尚不存在,旧的递归调用会把整棵树补齐,新的则不会。Risk 一节已经隐约提到了这点,但把它描述成「将来某个调用方传入更深的 lockPath」这种假设情形——而 QWEN_HOME 让同一个缺口在今天、不需要任何新调用点就能被触达。这值得改一下措辞,但不值得改代码:结果仍然是预期中的优雅跳过(记录失败、下一轮重试、链条完整),housekeeping 本身就是 best-effort 的清理;而且我没有找到其它会在启动时递归创建全局 qwen 目录的生产代码——唯一的递归创建者是 PollingChannelBase,而那是 getGlobalQwenDir()/channels,且取决于是否真的有 polling channel 在跑。

这段注释对得起它占的篇幅。 一行改动配十行注释通常是过头了,但这里的 why 确实不明显、而且是承重的:没有它,下一个贡献者会把 recursive: true 当成一个看起来显然更健壮的改进加回去,从而悄悄把这个挂死重新引入。它选择扩写上方原有注释而不是替换掉,也是对的。

测试是真护栏,不是摆设。 第一个新测试用的是一个确实不存在的目录,所以它钉住了「去掉 recursive 没有把创建能力一起丢掉」;而且断言 stat.mode & 0o077 === 0 而不是精确 mode,让这个 0o700 约定检查不受 umask 影响,是个很好的处理。第二个测试遍历每一次 mkdir 调用并拒绝 recursive: true;作者演示过把该选项加回去后它会以具体的 matcher 信息失败,所以它是可证伪的,而不是同义反复。

两个测试都没有钉住事故本身——「在源目录被删除的 bind mount 上 promise 永不 settle」需要 root 和 mount namespace,这对单元测试来说合理地属于范围之外。这个缺口正是下面那条验证建议要覆盖的。

复用方面没有问题:这里没有新增任何抽象,所以不存在需要合并的平行工具函数。

测试证据

更新:这一节首次写下之后 CI 已经落定,而且落定的结果是单元测试套件根本没跑起来。 本评论最初版本把 Linux 单元测试 job 报为进行中;下面的表格和分析是针对 commit 4d54e6a3364a462f046d424120ded6a9ba18f667 的最终状态。这是无人值守的 CI 运行,所以我没有构建、运行或执行本 PR 的任何代码——全部内容都通过 API 读取。55 个 check-run 中:16 个 success、2 个 failure、34 个 skipped、3 个仍在进行(bot 自己的 review-prtriage 编排任务,它们跑在 pull_request_target 上,不属于本 PR 的 CI)。

两个红叉都是 runner 侧的 checkout 失败,与本 PR 无关。 它们是同一类故障,而且都没有接触到 PR 的任何代码:

  • Classify PR 失败在它的 Checkout trusted CI classifier 步骤——该 job 中唯一非 success 的步骤——报错是 The process '/usr/bin/git' failed with exit code 128
  • Test (ubuntu-latest, Node 22.x) 失败在它的 Checkout 步骤,重试三次后报错 fatal: unable to access 'https://github.com/QwenLM/qwen-code/': Failed to connect to github.com port 443 after 134983 ms: Couldn't connect to server。测试步骤从未开始。

一个只改了 packages/cli/src/utils/housekeeping/ 下两个文件的变更,不可能让自建 runner 连不上 github.com:443。这个结论是基于失败的步骤身份和 git/网络报错文本得出的——两者都由 runner 产生——而不是基于日志正文里的任何说法。

但「基础设施噪音」不等于「证据已经存在」,而这个区别决定了此处的结论。 唯一会执行本 PR 两个新测试的 job 在跑任何一个测试之前就死了,而本 PR 的 macOS 与 Windows Test 矩阵都是 skipped。所以在 CI 里,没有任何平台运行过 creates the lock directory when it does not exist yetnever asks for a recursive mkdir。真正通过的部分是有价值且实在的——Lint & Static(说明改动文件的 typecheck 和 ESLint 是干净的)、Integration Tests (no-AK, No Sandbox)、OpenTUI no-flicker gate、两个 Desktop Shell 任务、TUI parity snapshots——但它们都不接触 throttledOnce。这两个新测试目前除了作者自己的机器之外,在任何地方都没有被执行过。

明确说明未验证的部分:

  • 两个新单元测试没有任何 CI 结果。 Linux job 在 Checkout 就失败了;macOS/Windows 是 skipped。关于它们唯一存在的数字是作者提供的(macOS 上 throttledOnce.test.ts 13 项通过、housekeeping 全量 80 项通过)——那是作者的自述,不是证据,此处也未重跑。
  • bind mount 复现是作者提供的证据,未经独立重跑。 before/after 数据(递归形式 6 秒后仍 pending、CPU 103%,对比单层形式 ENOENT、CPU 0%)、strace 特征,以及集群实测数据(177 个进程、每秒约 2500 次失败 mkdir、load average 213 → 12)全部来自 PR 描述。复现它们需要 root 和 mount namespace,而本次运行既不执行 PR 派生代码,也不具备该环境。
  • Windows 未测试——作者标注 ⚠️,理由是该故障模式为 Linux bind mount 特有、而改动本身与平台无关。这个推理看起来成立(不带 recursivemkdir 并非平台特有 API),但由于 Windows Test 矩阵是 skipped,这里也无法证实或证伪。
  • 作者自己提出的疑问仍然是疑问: 为什么同一个永不 settle 的调用,在部分进程中表现为 mkdir 热循环、在另一些进程中表现为完全没有 mkdir 系统调用的安静挂起。无论如何本 PR 会同时消除这两种表现,所以这不阻塞改动——但这是一个诚实的未知项,而且被如实标注为未知,没有被强行解释掉。

沙箱化验证可以搞定目前唯一还只依赖作者自述的论断:@qwen-code /verify —— 即在源目录被删除的挂载点上,mkdir(..., { recursive: true }) 是否真的不会 settle,以及去掉该选项后是否快速拒绝,与 base build 做 A/B。本 PR 自己的套件钉住了不变量(没有任何 mkdir 调用请求 recursive: true),但没有钉住让这个不变量重要的机制,而 mount namespace 复现正是静态审查无法替代的那一类东西。顺带一提,它也会让这两个新测试第一次真正被执行。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 4d54e6a3364a462f046d424120ded6a9ba18f667 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — I'm confident in the code and would merge it as written; what I can't do is approve against a commit where the only job that would have run this PR's two new tests died at Checkout and never executed a single one. That's a CI re-run, not an author change.

Stepping back over the whole picture: this is what a well-formed small fix looks like. I wrote down my own proposal from the title and the "Why it's needed" section before reading the diff, and the PR landed on exactly it — drop recursive: true so the call fails fast instead of retrying forever. The two alternatives I considered are both worse, and worth saying why: a timeout around the mkdir lets the awaiting chain move on but can't cancel the retry loop Node is already running, so the CPU cost that turned this into an incident would likely survive it; pre-statting the parent adds code without removing the unbounded retry at all. One line removes both symptoms. There's nothing to cut here — no drive-by refactor, no formatting churn, no scope creep, and the 38 lines of test are proportionate to a change whose whole point is an invariant that's easy to accidentally undo.

What I'd have wanted to check going in was whether "one level is enough" was actually true, because that's the single assumption the fix rests on, and it's the kind of claim that reads plausible in a PR description and turns out to have a fourth call site nobody remembered. It holds. All three call sites put lockPath directly in the global qwen dir — including the two that hash a key into the marker filename, which was the plausible place for a hidden extra directory level and isn't. I also followed the throw path when the mkdir does fail, on both schedulers, and both catch it and reschedule, so the change converts a hang into the graceful skip it claims to produce rather than into a new crash.

On the problem itself: this is about as far from theoretical hardening as a fix: PR gets. There's a production incident with fleet-level measurements, an strace signature from a live wedged process, a self-contained reproduction, and a cross-reference to #11264 that I verified is real, open, and genuinely the complementary half rather than a duplicate. The author also flags their own unresolved question — why the same never-settling promise spins a core in some processes and hangs quietly in others — instead of papering over it with a confident-sounding explanation. That's the opposite of the pattern this gate exists to catch, and it made me trust the rest of the description more, not less.

So why not approve. Two reasons, and they're different in kind:

  • The test evidence doesn't exist yet. Test (ubuntu-latest, Node 22.x) failed in its Checkout step — Failed to connect to github.com port 443 after ~135s, three attempts — so the test step never started. macOS and Windows Test matrices are skipped on this PR. That means creates the lock directory when it does not exist yet and never asks for a recursive mkdir have never run in CI on any platform. Everything else is green and some of it is meaningful (Lint & Static clears typecheck and ESLint on the changed file; integration tests, both Desktop Shell jobs, the OpenTUI gate and TUI parity all pass), but none of it executes throttledOnce. Approving now would attest to a unit-test result that does not exist, which is the exact thing I shouldn't do — and "the red is infra noise" explains why the evidence is missing without supplying it.
  • Both red checks are genuinely not this PR's fault, and I want to be explicit that I'm not asking the author to fix anything. Classify PR failed in Checkout trusted CI classifier with git exit 128; the unit job failed reaching github.com:443. A two-file change under packages/cli/src/utils/housekeeping/ cannot cause either. I judged that from the failing step identities and the git/network error text, both runner-generated — not from claims in the log body.

⏸️ Deferring, not requesting changes. What this needs is a re-run of the Test (ubuntu-latest, Node 22.x) job on this same commit. Once that job goes green on 4d54e6a3364a462f046d424120ded6a9ba18f667, this is approvable exactly as it stands — I would not ask for a single code change, and re-running /triage at that point should approve it.

No approval marker is attached to this comment: the PR's own CI workflow runs have all completed (PENDING = 0), so there is nothing left for a finalize job to wait on — the missing evidence has to come from a human re-triggering the failed job.

I'm also posting this without an @mention, which I'd normally avoid on a defer. The maintainer resolver found nobody accountable to hand this to: the PR carries no labels, so no area owner matched, there's no prior human reviewer to fall back to, and no maintainer handle is configured in this run. Flagging it here rather than guessing a login. Whoever picks it up: the ask is one re-run, not a review.

One optional, non-blocking follow-up for the author, already detailed above: the Risk section frames "a lockPath whose parent doesn't exist" as a hypothetical future caller, but QWEN_HOME makes the global qwen dir's own parent arbitrary today, so that gap is reachable now without any new call site. The behaviour is still the intended graceful skip, so this is a wording tweak to the risk note, not a code change — worth a sentence so the next reader doesn't have to rediscover it.

中文说明

信心度:3/5 —— 我对代码本身有信心,按现在的样子我就会合并;我做不到的是,在一个「唯一会执行本 PR 两个新测试的 job 死在 Checkout、一个测试都没跑」的 commit 上给出 approve。这需要的是 CI 重跑,不是作者改代码。

退一步看整体:这是一个形态良好的小 fix 该有的样子。在读 diff 之前,我先根据标题和「为什么需要」那一节写下了自己的方案,而 PR 落点与之完全一致——去掉 recursive: true,让这个调用快速失败而不是无限重试。我考虑过的另外两条路都更差,值得说明原因:给 mkdir 包一层超时能让等待的一方继续走下去,但无法取消 Node 已经在跑的重试循环,所以真正把这件事酿成事故的那部分 CPU 开销很可能依然存在;先 stat 父目录则只是增加代码,完全没有消除无上限重试。一行改动同时消除两个症状。这里没有可砍的东西——没有顺手重构、没有格式化噪音、没有范围蔓延,而 38 行测试相对于一个「核心就是一个容易被无意撤销的不变量」的改动是相称的。

我一开始最想核对的是「只创建一层就够了」是否真的成立,因为这是整个 fix 所依赖的唯一假设,也正是那种在 PR 描述里读起来很合理、结果却存在第四个没人记得的调用点类型的论断。它成立。三个调用点都把 lockPath 直接放在全局 qwen 目录下——包括那两个把 key 哈希进 marker 文件名的调用点,那本来是最可能藏着额外目录层级的地方,而并没有。我也顺着 mkdir 真的失败时的抛出路径,在两个 scheduler 上都走了一遍,两边都接住了并重新排程,所以这个改动是把挂死转成了它所声称的优雅跳过,而不是转成一个新的崩溃。

关于问题本身:这大概是 fix: 类 PR 里离「理论性加固」最远的一种。有带集群级实测数据的生产事故、有从活体卡死进程抓到的 strace 特征、有自包含的复现,还有一个我核对过确实存在、处于 open、并且真的是互补而非重复的 #11264 交叉引用。作者还主动标出了自己未解决的疑问——为什么同一个永不 settle 的 promise 在部分进程里烧掉一个核、在另一些进程里安静挂起——而不是用一个听起来很自信的解释把它糊过去。这与本关卡存在的目的所要拦截的那类模式恰好相反,而且它让我更信任描述的其余部分,而不是更不信任。

那么为什么不 approve。两个理由,性质不同:

  • 测试证据还不存在。 Test (ubuntu-latest, Node 22.x) 失败在它的 Checkout 步骤——Failed to connect to github.com port 443 after ~135s,重试三次——所以测试步骤从未开始。本 PR 的 macOS 与 Windows Test 矩阵是 skipped。这意味着 creates the lock directory when it does not exist yetnever asks for a recursive mkdir 在 CI 里、在任何平台上都从未运行过。其余全绿,其中一部分是有意义的(Lint & Static 说明改动文件的 typecheck 与 ESLint 干净;集成测试、两个 Desktop Shell 任务、OpenTUI gate、TUI parity 都通过),但它们都不执行 throttledOnce。现在 approve 等于为一个并不存在的单元测试结果背书,而这正是我不该做的事——「红叉是基础设施噪音」解释了证据为什么缺失,但并不等于提供了证据。
  • 两个红叉确实都不是本 PR 的错,我想明确说明我并没有要求作者修任何东西。Classify PR 失败在 Checkout trusted CI classifier,git exit 128;单元测试 job 失败在连不上 github.com:443。一个只改了 packages/cli/src/utils/housekeeping/ 下两个文件的变更不可能造成其中任何一个。我是基于失败的步骤身份和 git/网络报错文本判断的,两者都由 runner 产生——不是基于日志正文里的说法。

⏸️ 暂缓(defer),不是要求修改(request changes)。 这里需要的是在同一个 commit 上重跑 Test (ubuntu-latest, Node 22.x)。一旦该 job 在 4d54e6a3364a462f046d424120ded6a9ba18f667 上变绿,这个 PR 按现状即可 approve——我不会要求改动任何一行代码,届时重跑 /triage 就应该会通过。

本评论不附带 approve 标记:本 PR 自己的 CI workflow run 已全部完成(PENDING = 0),所以没有剩下什么可供 finalize 任务等待——缺失的证据必须由人重新触发那个失败的 job 才能补上。

我这条 defer 也没有加 @提及,这通常是我会避免的。维护者解析器没有找到可以交接的负责人:该 PR 没有任何标签,所以没有匹配到领域 owner;没有在此之前的真人 reviewer 可供回落;本次运行也没有配置维护者 handle。与其猜一个登录名,不如在这里如实说明。接手的人请注意:需要做的是一次重跑,不是一次 review。

另有一条给作者的、可选且不阻塞的后续项,上文已详述:Risk 一节把「父目录不存在的 lockPath」描述成假设中的未来调用方,但 QWEN_HOME 使得全局 qwen 目录自身的父目录在今天就是任意的,所以这个缺口现在无需任何新调用点即可被触达。行为仍然是预期中的优雅跳过,所以这是对风险说明的措辞调整,不是代码改动——值得写一句,好让下一个读者不必重新发现它。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 4d54e6a3364a462f046d424120ded6a9ba18f667 · re-run with @qwen-code /triage

@doudouOUC

Copy link
Copy Markdown
Collaborator Author

@qwen-code /takeover

@doudouOUC
doudouOUC enabled auto-merge September 7, 2026 08:34
@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Sep 7, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Test Plan (not a blocker): 13 passed — this review observed 28911 passed; 80 passed — this review observed 28911 passed.

中文说明

仅完成部分审查,审查缺口已披露。

未审查(原文为英文):build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Test Plan(非阻断):13 passed — this review observed 28911 passed; 80 passed — this review observed 28911 passed

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/utils/housekeeping/throttledOnce.test.ts Outdated
Comment thread packages/cli/src/utils/housekeeping/throttledOnce.test.ts

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Not reviewed: reverse audit — stopped before round 3 by the review time budget.

Test Plan (not a blocker): 13 passed — this review observed 28910 passed; 80 passed — this review observed 28910 passed.

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/utils/housekeeping/throttledOnce.ts:62 — [probe] comment's '$HOME' premise false under QWEN_HOME
  • packages/cli/src/utils/housekeeping/throttledOnce.ts:63 — [probe] swallowed mkdir error is never logged
中文说明

仅完成部分审查,审查缺口已披露。

未审查(原文为英文):build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

未审查:反向审计——评审时间预算不足,未能开始第 3 轮。

Test Plan(非阻断):13 passed — this review observed 28910 passed; 80 passed — this review observed 28910 passed

收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/utils/housekeeping/throttledOnce.test.ts Outdated
Comment thread packages/cli/src/utils/housekeeping/throttledOnce.test.ts
@qwen-code-dev-bot

qwen-code-dev-bot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

AutoFix round 1 finishedview run. See this round's report below.

中文说明

AutoFix 第 1 轮已完成 —— 查看运行。本轮报告见下方。

@wenshao

wenshao commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Local runtime verification

I built both arms and ran this against a real deleted bind mount instead of reading the diff. Verdict: merge as it stands — every claim in the description reproduces, the two unit tests CI never got to run pass on Linux and have teeth, the ⚠️ Windows row is now measured, and the one question the description leaves open has a concrete answer. One out-of-scope follow-up is worth filing separately.

Harness

  • Two real npm run bundle CLI builds: base 9a954b4198 (the PR parent) and head 4d54e6a336. The bundled runThrottledOnce differs between them at exactly one call and nothing else:
    -  await mkdir(dirname(opts.lockPath), { recursive: true, mode: 448 }).catch(() => {});
    +  await mkdir(dirname(opts.lockPath), { mode: 448 }).catch(() => {});
  • A private mount namespace (unshare -m) holding a bind mount whose source directory is deleted, as root.
  • Real win32 node.exe v22.22.2 (win-x64) under Wine for the platform row.
  • Stated up front: everything ran as uid 0; one code-generated web-template file was stubbed identically in both bundles (unrelated to this path, needed only to make the bundle link); the Wine probe bundles throttledOnce.ts with the debug logger shimmed; Wine is not Windows.

1. The mechanism reproduces exactly as described

mechanism

recursive: true was still pending at 6 s at 94 % CPU; the single-level form rejected ENOENT in 0 ms at 0 %. The strace confirms the loop the description reasons about, verbatim: mkdir(leaf)=ENOENTmkdir(parent)=EEXISTstatx(parent)=S_IFDIR → retry leaf, ~10k failing mkdir/s, no cap, no backoff. Two extras worth recording: fs.mkdirSync(…, { recursive: true }) wedges identically (99 % CPU, main thread), and a single wedged chain does not meaningfully starve other fs work in the process (readFile p50 × 1.3) — so the cost really is CPU, not I/O latency.

2. The real CLI in the production sequence — the decisive A/B

The incident shape is a process that started healthy and had the mount deleted underneath it, so that is what I reproduced: launch the real TUI with QWEN_HOME inside the bind mount, let it bootstrap its qwen dir, rm -rf the mount source at t+14 s, and let the housekeeping first pass fire at ~t+62 s.

real cli a/b

Both builds in one wedged mount, sampled with top at t+90 s: 91.2 % CPU for pre-PR against 0.0 % for this PR, same instant, same mount. Full 2×2, un-instrumented, CPU consumed between t+12 s and t+120 s:

build mount CPU / 108 s markers process
pre-PR intact 0.27 s (0 %) 3/3 alive
pre-PR source deleted 54.51 s (50 %, 94 % while spinning) 0/3 alive, wedged
this PR intact 0.25 s (0 %) 3/3 alive
this PR source deleted 0.24 s (0 %) 0/3 alive

Only the (pre-PR, deleted) cell burns CPU; the fix costs nothing in the healthy cell, and the TUI is identical in both arms afterwards — no error banner, no crash. I re-ran the whole 2×2 a second time and every cell reproduced to within 0.03 s.

syscall degradation

At syscall level over the same 50 s window: 105,390 failing mkdir and zero lock acquisitions on base, against 1 mkdir then one openat of the lock file on head. The head arm's throw lands in runPass's catch in 0.6 ms and the timer chain survives, which is exactly the graceful skip the PR claims. On base the first runThrottledOnce never returns, so the serial chain never reaches step 2 or 3 and runPass never schedules another pass — the "wedges the housekeeping chain for the life of the process" claim, observed rather than argued.

3. The two unit tests, run and mutation-tested

tests and teeth

throttledOnce.test.ts 13/13 on Linux. Full housekeeping surface: head 78 passed / 2 failed (80), base 76 passed / 2 failed (78) — the same two failures on both arms, the cleanup.test.ts EACCES cases, which cannot fail for uid 0. Environmental, not this PR.

Mutation results — 3 of 3 targeted mutants caught, working tree restored clean after each:

mutant outcome
restore { recursive: true, mode: 0o700 } never asks for a recursive mkdir
remove the mkdir(…) call entirely ✗ both new tests
widen the mode to { mode: 0o755 } creates the lock directory when it does not exist yet
add a second, options-less mkdir(dir) 13 passed — the guard loop does not false-positive

The stat.mode & 0o077 assertion has teeth of its own, and the guard loop tolerates a future options-less mkdir rather than erroring on it.

4. Windows — the ⚠️ row, measured

platform and quiet hang

Real win32 Node under Wine, driving each arm's compiled runThrottledOnce, with the same driver re-run natively on Linux:

case pre-PR this PR
parent exists, lock dir missing completed, dir created completed, dir created
whole parent chain missing completed, tree bootstrapped THREW ENOENT, nothing created
lock dir already exists (EEXIST) completed completed

win32 and linux produced identical results on both arms, marker written and no lock file leaked in every passing case. Your platform-neutrality reasoning holds up under measurement.

5. Your open question — a concrete answer

You wrote that you could not explain why the same never-settling call shows up as a hot mkdir loop in some processes and a quiet hang with no mkdir syscalls at all in others (promise pending, 0 % CPU, idle libuv workers). The CLI re-execs itself, so the pid you first reach for is not the pid running the code. One single pre-PR run in the wedged mount, observed from both vantage points at once:

launcher pid 3311163  (what ps/top/pgrep show first)    0.00s CPU / 100s    0 mkdir syscalls
re-exec'd worker 3311177 (where housekeeping runs)     46.82s CPU / 100s   26,759 mkdir in a 3s sample

I hit this myself: my first three runs reported 0.00 s CPU and zero mkdir syscalls and I was about to write up a "quiet variant" until I followed the child. That reproduces your description exactly. (A second, independent mechanism also produces it: saturate all four libuv threadpool threads and the recursive mkdir never gets a thread — verified pending at 10 s, 0 syscalls, 0 % CPU. Neither is a blocker; both disappear with this PR.)

6. On the review note about QWEN_HOME

The reachability concern raised in review does not hold in practice, and it is worth recording why rather than rewording the risk note on faith. writeOutputLanguageFile (packages/cli/src/i18n/languageUtils.ts:238) does fs.mkdirSync(dir, { recursive: true }) on the global qwen dir straight from main(), before the UI renders. Measured: QWEN_HOME=/deep/a/b/c/qwen with none of a, b, c existing is fully bootstrapped within seconds — in both interactive and -p non-interactive runs, ~60 s before the first housekeeping pass. So the dir housekeeping wants always exists by then, and your risk note is accurate as written (a statement about a hypothetical future caller). No change needed.

7. One follow-up, explicitly not this PR

startup wedge follow-up

If the container starts after the workspace was deleted (qwen dir never created inside the broken mount), both builds hang before rendering anything, at 100 % of one core (25.00 s CPU / 25 s wall, ~59k failing mkdir/s on the main thread). Tapping fs.mkdirSync via --require and reading the stack pins it to the same writeOutputLanguageFile call above; dist-patching installationManager.writeInstallationIdToFile and saveSettings to non-recursive did not stop the spin, because languageUtils gets there first. Both arms are identical here, so this is neither caused nor claimed by this PR — but it is the same bug class, and packages/{core,cli}/src has 226 recursive-mkdir call sites, 120 of them mkdirSync. Worth a follow-up issue (a shared "create one level, fail fast" helper), not a change here.

Not verified

macOS (the author's numbers stand unre-run); the non-interactive scheduler's degradation path (read only — the interactive one is measured end to end); the fleet-level incident figures (177 processes, 45 CPU-hours, load average 213 → 12); Windows only through Wine, not on a real Windows host.

中文说明

本地真实环境验证

我把两侧都构建出来,在一个真实的「源目录已删除的 bind mount」上跑,而不是只看 diff。结论:可以按现状合并 —— 描述里的每一条都复现了;CI 一个都没跑到的两个单元测试在 Linux 上通过且是真护栏;平台表里的 ⚠️ Windows 行现在有实测数据了;描述里唯一悬着的疑问也有了明确答案。另有一个超出本 PR 范围的后续项,值得单独开 issue。

验证环境

  • 两个真实的 npm run bundle CLI 构建:base 9a954b4198(PR 的父提交)与 head 4d54e6a336。两者打包后的 runThrottledOnce 在一个调用处不同,别无其它:
    -  await mkdir(dirname(opts.lockPath), { recursive: true, mode: 448 }).catch(() => {});
    +  await mkdir(dirname(opts.lockPath), { mode: 448 }).catch(() => {});
  • 私有 mount namespace(unshare -m),内含一个源目录已被删除的 bind mount,以 root 运行。
  • 真实 win32 node.exe v22.22.2(win-x64)跑在 Wine 上,用于补齐平台行。
  • 先把口径讲清楚:全部以 uid 0 运行;有一个代码生成的 web-template 文件在两个构建里用了完全相同的 stub(与本路径无关,只为让 bundle 能链接过去);Wine 探针把 throttledOnce.ts 单独打包并 shim 掉了 debug logger;Wine 不等于 Windows。

1. 机制与描述完全一致

recursive: true 在 6 秒后仍 pending,CPU 94%;单层形式 0 毫秒就以 ENOENT 拒绝,CPU 0%strace 逐字印证了描述中推演的循环:mkdir(叶子)=ENOENTmkdir(父)=EEXISTstatx(父)=S_IFDIR → 重试叶子,每秒约 1 万次失败的 mkdir,无上限、无退避。另外两条值得记录:fs.mkdirSync(…, { recursive: true }) 会以同样方式卡死(CPU 99%,且卡在主线程);单个卡死的调用链不会明显拖慢进程内其它 fs 操作(readFile p50 仅 ×1.3)—— 所以代价确实是 CPU,而不是 I/O 延迟。

2. 真实 CLI 走生产时序 —— 决定性的 A/B

事故形态是「进程启动时一切正常,之后挂载在它脚下被删掉」,所以我就照这个复现:把 QWEN_HOME 指到 bind mount 内启动真实 TUI,等它把 qwen 目录初始化好,在 t+14 秒 rm -rf 挂载源,然后等 housekeeping 首轮在约 t+62 秒触发。

两个构建放在同一个已损坏的挂载里,t+90 秒用 top 采样:改动前 91.2% CPU,本 PR 0.0%,同一时刻、同一挂载。完整 2×2(无 strace 干扰),统计 t+12 秒到 t+120 秒之间消耗的 CPU:

构建 挂载 CPU / 108 秒 marker 进程
改动前 完好 0.27 s(0%) 3/3 存活
改动前 源已删除 54.51 s(50%,空转期间 94%) 0/3 存活但卡死
本 PR 完好 0.25 s(0%) 3/3 存活
本 PR 源已删除 0.24 s(0%) 0/3 存活

只有「改动前 + 已删除」这一格在烧 CPU;修复在健康那格没有任何开销,事后两侧 TUI 完全一致 —— 没有报错横幅、没有崩溃。整个 2×2 我跑了两轮,每一格都复现到 0.03 秒以内。

同一 50 秒窗口的系统调用层面:base 侧 105,390 次失败 mkdir、0 次加锁;head 侧 1 次 mkdir 之后紧接着 1 次对锁文件的 openat。head 侧的抛出在 0.6 毫秒内落进 runPass 的 catch,定时器链存活,正是 PR 声称的优雅跳过。base 侧第一个 runThrottledOnce 永不返回,串行链条压根到不了第 2、3 步,runPass 也再不会安排下一轮 —— 「把整条 housekeeping 链卡死到进程生命周期结束」这一点是观测到的,而不是推理出来的。

3. 两个单元测试:真跑了,并做了变异测试

throttledOnce.test.tsLinux 上 13/13 通过。housekeeping 全量:head 78 通过 / 2 失败(80),base 76 通过 / 2 失败(78)—— 两侧是同样的两个失败,即 cleanup.test.ts 的 EACCES 用例,它们在 uid 0 下不可能失败。属于环境问题,与本 PR 无关。

变异测试结果 —— 3 个针对性变异全部被抓住,每次之后工作区都恢复干净:

变异 结果
加回 { recursive: true, mode: 0o700 } never asks for a recursive mkdir
整个删掉 mkdir(…) 调用 ✗ 两个新测试都失败
把权限放宽到 { mode: 0o755 } creates the lock directory when it does not exist yet
额外加一个不带 options 的 mkdir(dir) 13 通过 —— 护栏循环不会误报

stat.mode & 0o077 这条断言本身也有牙齿;而且护栏循环对将来出现的「不带 options 的 mkdir」是容忍而非报错。

4. Windows —— 补齐 ⚠️ 那一行

真实 win32 Node 跑在 Wine 上,驱动两侧编译产物中的 runThrottledOnce,同一个 driver 再在 Linux 原生跑一遍:

场景 改动前 本 PR
父目录存在、锁目录缺失 completed,目录已创建 completed,目录已创建
整条父路径都缺失 completed,整棵树被补齐 THREW ENOENT,什么都没创建
锁目录已存在(EEXIST completed completed

win32 与 linux 在两侧都给出一致结果,所有通过的场景都写了 marker、也没有泄漏锁文件。你关于「改动与平台无关」的推理经实测成立。

5. 你提出的疑问 —— 一个明确答案

你说无法解释为什么同一个永不 settle 的调用,在部分进程里表现为 mkdir 热循环,在另一些进程里表现为安静挂起、完全没有 mkdir 系统调用(promise 仍 pending、CPU 0%、libuv worker 全空闲)。CLI 会 re-exec 自身,所以你第一时间拿到的那个 pid 并不是真正跑代码的那个。改动前的构建在已损坏挂载里跑一次,同时从两个视角观测:

launcher pid 3311163(ps/top/pgrep 首先给你的)      100 秒内 CPU 0.00s      mkdir 系统调用 0 次
re-exec 出来的 worker 3311177(housekeeping 在此)  100 秒内 CPU 46.82s     3 秒采样 26,759 次 mkdir

我自己就踩了这个坑:前三次运行都报 0.00 秒 CPU、零 mkdir 系统调用,我差点就写下「安静变体」的结论,直到顺着子进程追下去。这与你的描述完全吻合。(还有一个独立机制也能产生同样现象:把 libuv 线程池的 4 个线程全占满,递归 mkdir 连线程都拿不到 —— 已验证:10 秒后仍 pending、0 次系统调用、CPU 0%。两者都不构成阻塞;本 PR 会一并消除。)

6. 关于评审中提到的 QWEN_HOME

评审里提出的可达性担忧在实际中不成立,而且值得把原因记下来,而不是凭感觉改措辞。writeOutputLanguageFilepackages/cli/src/i18n/languageUtils.ts:238)直接在 main() 里对全局 qwen 目录执行 fs.mkdirSync(dir, { recursive: true }),发生在 UI 渲染之前。实测:QWEN_HOME=/deep/a/b/c/qwenabc 都不存在时,整棵树会在几秒内被补齐 —— 交互模式与 -p 非交互模式都是如此,比首轮 housekeeping 早约 60 秒。所以 housekeeping 需要的那个目录到时候一定已经存在,你 Risk 一节的措辞按原样也是准确的(它说的是一个假设中的将来调用方)。无需改动。

7. 一个后续项,明确不属于本 PR

如果容器是在工作目录被删除之后才启动(qwen 目录从未在损坏的挂载里创建过),两个构建都会在渲染任何东西之前挂死,占满一整个核(25 秒墙钟消耗 25.00 秒 CPU,主线程每秒约 5.9 万次失败 mkdir)。用 --require 挂钩 fs.mkdirSync 并读取调用栈,定位到的正是上面那个 writeOutputLanguageFile;把 installationManager.writeInstallationIdToFilesaveSettings 通过 dist-patch 改成非递归都止不住空转,因为 languageUtils 抢在前面。这里两侧完全一致,所以既不是本 PR 引入的、本 PR 也没有声称要修 —— 但它属于同一类缺陷,而 packages/{core,cli}/src 里共有 226 处递归 mkdir 调用点,其中 120 处是 mkdirSync。建议单独开 issue 跟进(比如提供一个统一的「只建一层、快速失败」helper),而不是在本 PR 里改。

未验证部分

macOS(作者给出的数字未重跑);非交互 scheduler 的降级路径(只读代码 —— 交互路径是端到端实测的);集群层面的事故数据(177 个进程、45 CPU 小时、load average 213 → 12);Windows 仅通过 Wine 验证,未在真实 Windows 主机上跑。

…ledOnce

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下:

Autofix round — PR #11285

Commit: 51c94b68ed test(cli): guard win32 mode assertion and pin ENOENT effect in throttledOnce (1 file, +27/−1, test-only).

Feedback dispositions

rc:3948439991 + rc:3948855419 — [Critical] R1-1: unguarded permission-bit assertion fails on the Windows lane — RESOLVED

Verified the claim against in-repo evidence before editing (no win32 runner exists here either, so this mirrors the reviewer's settling authorities):

  • test_windows (.github/workflows/ci.yml:1641) runs npm run test:ci and is gated on merge_group || schedule || workflow_dispatch — not pull_request — so this PR's own CI stays green and the lane goes red in the merge queue/nightly, exactly as the finding describes.
  • packages/cli/package.json maps test:ci to vitest run, and packages/cli/vitest.config.ts excludes only node_modules/dist/cypress — so throttledOnce.test.ts runs on that lane.
  • Every pre-existing lane-reaching assertion of this shape is guarded: packages/core/src/config/storage.test.ts:770, packages/cli/src/serve/conversations/conversation-workspace.test.ts:110, packages/cli/src/serve/managed-scratch-workspace.test.ts:75 — each wraps mode & 0o077 in if (process.platform !== 'win32') with the same libuv rationale. This diff added the first unguarded one.

Fix: wrapped only the expect(stat.mode & 0o077).toBe(0) assertion in if (process.platform !== 'win32'), keeping the umask-proof comment and adding the libuv rationale, per the storage.test.ts precedent. The guard was deliberately NOT widened to it.skipIf over the whole case, so expect(stat.isDirectory()).toBe(true) and expect(result).toEqual({ status: 'completed' }) keep witnessing a one-level mkdir on Windows.

rc:3948440012 + rc:3948855430 — [Suggestion] R1-2: pin the effect of a never-creatable lock dir, not just the mkdir options — RESOLVED

Added surfaces ENOENT instead of bootstrapping a missing parent beside the two new tests. It uses the real filesystem (the file's vi.mock('node:fs/promises', { spy: true }) is call-through; nothing is stubbed), points lockPath at a directory whose own parent does not exist, and pins a rejection with code: 'ENOENT' plus task never called. The existing option-shape test never asks for a recursive mkdir is kept untouched — the two pin different regressions.

One deviation from the reviewer's literal snippet, forced by physics: the round-1 code block placed the lockPath ONE level below tempDir (tempDir/nope/.marker.lock), but the non-recursive mkdir(tempDir/nope) succeeds when tempDir exists, so that shape resolves completed instead of rejecting — inconsistent with the finding's own witness ("PRISTINE: REJECTED code=ENOENT"). The round-2 text describes the probe as "lockPath two levels below tempDir", which is what I implemented (tempDir/nope/nested/.marker.lock) and measured locally on Node v22: one-level mkdir RESOLVED, two-level mkdir REJECTED ENOENT, two-level recursive mkdir RESOLVED, open in a missing dir REJECTED ENOENT.

Failed check: Classify PR — not addressable from this PR

classify_pr (.github/workflows/ci.yml:107) is CI routing/classification machinery (release-sync detection, runner picking, CI profile) with continue-on-error: true. Its failure cannot be caused by this PR's two-file change, the fix would live in .github/ which is outside what this round may touch, and I have no GitHub credentials to read its logs. Left for the maintainers; flagged here so it is not silently dropped.

Reviewer's self-deferred probes in rv:5130934729 — left deferred, deliberately

throttledOnce.ts:62 comment's $HOME premise under QWEN_HOME, and throttledOnce.ts:63 swallowed mkdir error never logged. The reviewer itself recorded these as "Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round", so they live in the review ledger and will resurface when the posture lifts. Neither is a correctness defect; touching them unrequested would grow the diff against this window's minimal-fix guidance. Not silently dropped — stated here explicitly.

Mutation probes (each run, then restored; suite re-confirmed green after restore)

  • Guard-not-inert (R1-1's request): dropped mode: 0o700 from the mkdir at throttledOnce.ts:63creates the lock directory when it does not exist yet went RED (umask 022 → 0o755 & 0o077 = 0o055), 13 others passed. Restored.
  • MUTANT B (R1-2's request): relaxed tryAcquire's throw e to return false for non-EEXIST → surfaces ENOENT instead of bootstrapping a missing parent went RED (resolves { status: 'locked' } instead of rejecting ENOENT) while the other 13 stayed green — exactly the hole the finding identified. Restored.
  • MUTANT C: restored recursive: true at throttledOnce.ts:63 → both never asks for a recursive mkdir and the new effect test went RED (recursive mkdir bootstraps the parent, so the run resolves completed), 12 others passed. This is the parent-bootstrapping reroute measured through Node's own recursive mkdir. Restored.

Verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • cd packages/cli && npx vitest run src/utils/housekeeping/throttledOnce.test.ts — 14 passed (13 pre-existing + 1 new)
  • cd packages/cli && npx vitest run src/utils/housekeeping/ — 41 passed, 2 files (throttledOnce + scheduler)

Settings schema untouched (no settings source changed); integration tests not applicable (behavior is fully exercised by unit tests; no bundled-CLI-only surface).

中文说明

Autofix 本轮处理 — PR #11285

提交:51c94b68ed test(cli): guard win32 mode assertion and pin ENOENT effect in throttledOnce(1 个文件,+27/−1,仅测试)。

反馈处置

rc:3948439991 + rc:3948855419 —— [Critical] R1-1:未加保护的权限位断言会在 Windows lane 上失败 —— 已解决

修改前先依据仓库内证据核验了该论断(本环境同样没有 win32 runner,因此采用与评审者相同的定论依据):

  • test_windows.github/workflows/ci.yml:1641)运行 npm run test:ci,且触发条件为 merge_group || schedule || workflow_dispatch —— 不含 pull_request —— 所以本 PR 自己的 CI 保持绿色,而该 lane 会在 merge queue / nightly 上变红,与发现描述完全一致。
  • packages/cli/package.jsontest:ci 映射为 vitest run,且 packages/cli/vitest.config.ts 只排除 node_modules/dist/cypress —— 因此 throttledOnce.test.ts 会在该 lane 上执行。
  • 既有的、会跑到该 lane 的同形状断言全部有保护:packages/core/src/config/storage.test.ts:770packages/cli/src/serve/conversations/conversation-workspace.test.ts:110packages/cli/src/serve/managed-scratch-workspace.test.ts:75 —— 每一处都把 mode & 0o077 包在 if (process.platform !== 'win32') 里并附有相同的 libuv 说明。本 diff 新增的是第一处未加保护的。

修复:仅把 expect(stat.mode & 0o077).toBe(0) 这一个断言包进 if (process.platform !== 'win32'),保留原有的 umask 说明注释并补充 libuv 缘由,与 storage.test.ts 的先例一致。保护刻意没有扩大成对整个用例的 it.skipIf,因此 expect(stat.isDirectory()).toBe(true)expect(result).toEqual({ status: 'completed' }) 在 Windows 上仍然见证「单层 mkdir 仍能创建锁目录」。

rc:3948440012 + rc:3948855430 —— [Suggestion] R1-2:钉住「锁目录永远无法创建」的效果,而不仅是 mkdir 的选项 —— 已解决

在两个新增测试旁边补了 surfaces ENOENT instead of bootstrapping a missing parent。该用例走真实文件系统(本文件的 vi.mock('node:fs/promises', { spy: true }) 是 call-through 的,没有任何桩),把 lockPath 指向一个父目录本身就不存在的目录,并钉住「以 code: 'ENOENT' 拒绝」以及 task 从未被调用。既有的选项形状测试 never asks for a recursive mkdir 原样保留 —— 两者钉住的是不同的退化。

与评审者字面代码片段的一处偏差(由实际行为强制):第 1 轮代码块把 lockPath 放在 tempDir 之下一层(tempDir/nope/.marker.lock),但 tempDir 存在时非递归的 mkdir(tempDir/nope) 会成功,因此该形状会 resolve 为 completed 而不是拒绝 —— 与该发现自己的证词(「PRISTINE: REJECTED code=ENOENT」)不一致。第 2 轮正文把探针描述为「lockPath 在 tempDir 之下两层」,我按此实现(tempDir/nope/nested/.marker.lock)并在 Node v22 上实测:一层 mkdir RESOLVED,两层 mkdir REJECTED ENOENT,两层递归 mkdir RESOLVED,在缺失目录中 open REJECTED ENOENT。

失败的检查:Classify PR —— 本 PR 无法处置

classify_pr.github/workflows/ci.yml:107)是 CI 路由/分类设施(release-sync 检测、runner 选择、CI profile),且带 continue-on-error: true。它的失败不可能由本 PR 的两个文件改动引起;修复只能落在 .github/,而那是本轮不允许触碰的区域;并且我没有 GitHub 凭据去读取它的日志。留给维护者处理 —— 在此明确标出,不做静默丢弃。

rv:5130934729 中评审者自我延后的两条探针 —— 刻意保持延后

throttledOnce.ts:62 注释的 $HOME 前提在 QWEN_HOME 下不成立;throttledOnce.ts:63 被吞掉的 mkdir 错误从不记录日志。评审者自己已将它们标记为「收敛姿态下延后(第 2 轮,非阻塞)—— 已记录,本轮不要求修改」,因此它们留在评审 ledger 里,姿态解除后会重新浮现。两者都不是正确性缺陷;在未被要求的情况下改动它们只会让 diff 违背本窗口的最小修复导向。不做静默丢弃 —— 在此明确说明。

变异探针(每条都已实际执行并随后还原;还原后套件重新确认为绿)

  • 保护未失效(R1-1 的要求):从 throttledOnce.ts:63mkdir 中去掉 mode: 0o700creates the lock directory when it does not exist yet 变红(umask 022 → 0o755 & 0o077 = 0o055),其余 13 个通过。已还原。
  • MUTANT B(R1-2 的要求):把 tryAcquirethrow e 放宽为对非 EEXIST 错误 return falsesurfaces ENOENT instead of bootstrapping a missing parent 变红(resolve 为 { status: 'locked' } 而不是以 ENOENT 拒绝),同时其余 13 个保持绿色 —— 正是该发现指出的漏洞。已还原。
  • MUTANT C:在 throttledOnce.ts:63 恢复 recursive: truenever asks for a recursive mkdir 与新增的效果测试同时变红(递归 mkdir 会补齐父目录,于是运行 resolve 为 completed),其余 12 个通过。这相当于通过 Node 自带的递归 mkdir 实测了「补齐父目录」的改道。已还原。

验证

  • npm run build —— 通过
  • npm run typecheck —— 通过
  • npm run lint —— 通过
  • cd packages/cli && npx vitest run src/utils/housekeeping/throttledOnce.test.ts —— 14 个通过(13 个既有 + 1 个新增)
  • cd packages/cli && npx vitest run src/utils/housekeeping/ —— 41 个通过,2 个文件(throttledOnce + scheduler)

未触碰 settings schema(未改动任何 settings 源);集成测试不适用(该行为已被单元测试完整覆盖,无仅经打包 CLI 才能触及的面)。

🦷 Gate advisory — this round resolves a Critical/Request-changes finding with test-only changes (machine-measured): the bite check cannot verify a test-side fix, so the resolution rests on the round summary alone. · 本轮以纯测试改动解决 Critical/Request-changes 反馈(门自动测量):bite 检查无法验证测试侧修复,该解决仅以轮次摘要为凭。

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

🧵 Resolved all 4 selected review thread(s). · 已关闭全部选中的 4 条评审线程。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 kimi-k3 · CLI 0.23.0

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope: packages/cli/src/utils/housekeeping/throttledOnce.ts, throttledOnce.test.ts. scheduler.ts read for cross-file caller verification (not in diff).

Tier: Standard — targeted bug fix changing one mkdir option; filesystem behavior boundary; caller coverage required.


No blocking findings.

Checked:

  1. All 3 callers of runThrottledOnce in scheduler.ts — every call site passes lockPath = join(qwenDir, '<marker>.lock') where qwenDir = Storage.getGlobalQwenDir(). So dirname(lockPath) = qwenDir, and the single-level mkdir targets only that one directory. Its parent is always a pre-existing directory (e.g. $HOME in the default case, or the parent of QWEN_HOME in a customized layout). The PR's claim that one level is sufficient is verified against all three callers.

  2. Error handling chainmkdir(qwenDir, { mode: 0o700 }).catch(() => {}): EEXIST is swallowed when the dir already exists (normal case); ENOENT is swallowed when the parent is a deleted bind mount. Either way the subsequent lock acquisition (open(lockPath, 'wx')) surfaces the filesystem error and runThrottledOnce propagates it, so housekeeping degrades to a skip — the documented intended behaviour.

  3. Three new tests:

    • 'creates the lock directory when it does not exist yet' — real-fs test; parent tempDir exists; single-level mkdir succeeds; permission bits asserted with correct win32 guard (if (process.platform !== 'win32')).
    • 'never asks for a recursive mkdir'vi.spy on node:fs/promises; inspects every mkdir call's options; guard is real (author confirms restoring recursive: true makes it fail).
    • 'surfaces ENOENT instead of bootstrapping a missing parent' — real-fs test; tempDir/nope/nested where nope doesn't exist; non-recursive mkdir swallowed, lock open then rejects with ENOENT → correct fast-fail.

Cross-check (findings frozen above before reading existing reviews):

  • CI bot R1-1 (CHANGES_REQUESTED, round 1 + round 2): "permission-bit assertion has no win32 guard"Refuted at current head (51c94b68). The autofix commit added if (process.platform !== 'win32') around the expect(stat.mode & 0o077).toBe(0) assertion. Confirmed in the current diff.

  • CI bot R1-2 (Suggestion): spy-options pin — present at head; suggestion-level; does not block.

  • CI bot deferred (round 2): $HOME comment accuracy under QWEN_HOME — confirmed minor comment imprecision, not a code defect; the actual behaviour (mkdir one level, fail-fast otherwise) is unchanged and acceptable. Swallowed mkdir error not logged — confirmed observability gap; non-blocking.

  • wenshao APPROVED with local runtime verification on a real deleted bind mount: claims reproduced, Linux + Windows unit suites pass. Strengthens the verdict.

Reviewed with AI assistance.

@doudouOUC
doudouOUC dismissed qwen-code-ci-bot’s stale review September 7, 2026 12:40

Already have 2 approves,3ks.

@doudouOUC
doudouOUC added this pull request to the merge queue Sep 7, 2026
Merged via the queue into QwenLM:main with commit f071516 Sep 7, 2026
61 of 62 checks passed
@doudouOUC
doudouOUC deleted the fix/mkdirp-stale-mount-hang branch September 7, 2026 12:44
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants